feat: configurable disk caching #2863

jackfrancis · 2020-03-09T18:54:36Z

Reason for Change:

This PR introduces configurable disk caching:

For OS disk configuration on master VMs:

either "ReadOnly", "ReadWrite", or "None"

For OS disk configuration on agent pool VMs:

either "ReadOnly", "ReadWrite", or "None"
For data disk configuration on agent pool VMs:
either "ReadOnly", "ReadWrite", or "None"
- supports a single configuration option across all data disks

An example api model w/ this new configuration:

{
...
        "masterProfile": {
            ...
            "osDiskCachingType": "ReadOnly",
            ...
        },
        "agentPoolProfiles": [
            {
            ...
            "osDiskCachingType": "ReadWrite",
            "dataDiskCachingType": "None",
            ...
            }
        ]
...
}

Issue Fixed:

Fixes #2249

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

jackfrancis · 2020-03-09T18:56:06Z

@PacoGuy How does the api model spec in the PR description look in terms of what you'd want in a configurable disk caching configuration?

And @mboersma FYI on intent, and @marosset to make sure we ship this w/ Windows 1st class support as well.

codecov · 2020-03-09T19:10:25Z

Codecov Report

Merging #2863 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
+ Coverage   71.13%   71.22%   +0.09%     
==========================================
  Files         147      147              
  Lines       25670    25738      +68     
==========================================
+ Hits        18261    18333      +72     
+ Misses       6272     6268       -4     
  Partials     1137     1137

Impacted Files	Coverage Δ
pkg/api/types.go	`94.37% <ø> (ø)`
pkg/api/vlabs/types.go	`73.30% <ø> (ø)`
pkg/api/converterfromapi.go	`93.64% <100.00%> (+0.03%)`	⬆️
pkg/api/convertertoapi.go	`93.15% <100.00%> (+0.03%)`	⬆️
pkg/api/defaults.go	`92.58% <100.00%> (+0.10%)`	⬆️
pkg/api/vlabs/validate.go	`80.15% <100.00%> (+0.88%)`	⬆️
pkg/engine/virtualmachines.go	`80.85% <100.00%> (+0.19%)`	⬆️
pkg/engine/virtualmachinescalesets.go	`80.93% <100.00%> (+0.14%)`	⬆️
pkg/engine/engine.go	`85.85% <0.00%> (+0.05%)`	⬆️
pkg/engine/armvariables.go	`86.17% <0.00%> (+0.08%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 414828f...89f1739. Read the comment docs.

PacoGuy · 2020-03-09T19:29:07Z

Looks like that PR model spec would work for us, Thanks!

marosset · 2020-03-09T21:48:41Z

@jackfrancis This should only add changes to the ARM correct?
If so I don't think anything specific needs to be done from windows (other than adding a test config or something to verify/validate)
LMK if you do need help tho.

stale · 2020-04-08T22:12:33Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jackfrancis · 2020-04-30T19:23:05Z

examples/e2e-tests/kubernetes/release/default/definition.json

@@ -121,7 +123,8 @@
        "vmSize": "Standard_D2s_v3",
        "osType": "Windows",
        "availabilityProfile": "VirtualMachineScaleSets",
-        "vnetSubnetId": "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME"
+        "vnetSubnetId": "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME",
+        "osDiskCachingType": "ReadOnly"


@marosset FYI, adding this new configuration to a Windows pool in our E2E cluster configuration to confirm that this is not a Linux/Windows thing.

Btw, should we add a data disk to this pool to (1) test that functionality generally, and to also validate the data disk caching type override?

mboersma

/lgtm

mboersma · 2020-04-30T19:44:50Z

pkg/api/vlabs/validate_test.go

+		{
+			name: "Master Profile with invalid OSDiskCachingType",
+			masterProfile: MasterProfile{
+				DNSPrefix:         "whizbang",


I thought "qux" followed "baz." 😜

There is no spoon.

acs-bot · 2020-04-30T19:49:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, mboersma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jackfrancis,mboersma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stale bot added the stale label Apr 8, 2020

stale bot closed this Apr 17, 2020

jackfrancis reopened this Apr 17, 2020

stale bot removed the stale label Apr 17, 2020

mboersma added needs-rebase Changes in the target branch require a `git rebase` and `git push -f` do-not-merge/work-in-progress labels Apr 22, 2020

jackfrancis force-pushed the configurable-disk-caching-type branch from 39e2820 to ba6756a Compare April 28, 2020 19:55

jackfrancis changed the title ~~[WIP] feat: configurable disk caching~~ feat: configurable disk caching Apr 28, 2020

mboersma removed do-not-merge/work-in-progress needs-rebase Changes in the target branch require a `git rebase` and `git push -f` labels Apr 29, 2020

jackfrancis added 4 commits April 30, 2020 09:38

feat: configurable disk caching

01ffaa0

actually implement

a5dda33

more UT updates

e23879e

test: update validate UT

9d49220

jackfrancis force-pushed the configurable-disk-caching-type branch from c70e730 to 9d49220 Compare April 30, 2020 16:39

acs-bot added size/L approved labels Apr 30, 2020

CecileRobertMichon mentioned this pull request Apr 30, 2020

Configurable OS disk caching kubernetes-sigs/cluster-api-provider-azure#584

Closed

test: update test configs

89f1739

jackfrancis commented Apr 30, 2020

View reviewed changes

mboersma approved these changes Apr 30, 2020

View reviewed changes

acs-bot assigned mboersma Apr 30, 2020

acs-bot added the lgtm label Apr 30, 2020

jackfrancis merged commit cfef30d into Azure:master Apr 30, 2020

jackfrancis deleted the configurable-disk-caching-type branch April 30, 2020 21:34

fmotrifork mentioned this pull request May 26, 2020

aks-engine 0.51.0 fishworks/fish-food#748

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: configurable disk caching #2863

feat: configurable disk caching #2863

jackfrancis commented Mar 9, 2020

jackfrancis commented Mar 9, 2020

codecov bot commented Mar 9, 2020 •

edited

Loading

PacoGuy commented Mar 9, 2020

marosset commented Mar 9, 2020

stale bot commented Apr 8, 2020

jackfrancis Apr 30, 2020

marosset Apr 30, 2020

mboersma left a comment

mboersma Apr 30, 2020

jackfrancis Apr 30, 2020

acs-bot commented Apr 30, 2020

feat: configurable disk caching #2863

feat: configurable disk caching #2863

Conversation

jackfrancis commented Mar 9, 2020

jackfrancis commented Mar 9, 2020

codecov bot commented Mar 9, 2020 • edited Loading

Codecov Report

PacoGuy commented Mar 9, 2020

marosset commented Mar 9, 2020

stale bot commented Apr 8, 2020

jackfrancis Apr 30, 2020

Choose a reason for hiding this comment

marosset Apr 30, 2020

Choose a reason for hiding this comment

mboersma left a comment

Choose a reason for hiding this comment

mboersma Apr 30, 2020

Choose a reason for hiding this comment

jackfrancis Apr 30, 2020

Choose a reason for hiding this comment

acs-bot commented Apr 30, 2020

codecov bot commented Mar 9, 2020 •

edited

Loading